Warn at runtime on degree coords + meter elevation in slope planar path#2777
Merged
Conversation
… path (#2764) The planar slope path uses coordinate spacing directly as the cell size. With degree (lat/lon) coordinates and meter elevation values, the result is wrong by orders of magnitude and nothing told the caller. Wire the existing warn_if_unit_mismatch helper into the planar branch so a UserWarning fires when the mismatch is detected. Add tests asserting the warning is emitted for degree coordinates and not emitted for projected/meter coordinates. Document the runtime warning in the slope docstring.
brendancol
commented
Jun 1, 2026
Contributor
Author
brendancol
left a comment
brendancol
left a comment
There was a problem hiding this comment.
PR Review: Warn at runtime on degree coords + meter elevation in slope planar path
Blockers (must fix before merge)
None.
Suggestions (should fix, not blocking)
- aspect() has the same planar/degree footgun and the same helper would apply there. Out of scope for this PR, but flagging it so it does not get forgotten.
Nits (optional improvements)
- xrspatial/slope.py: the warning only fires in the planar branch, which is correct since geodesic handles lat/lon properly. Noting it so the asymmetry is on the record as intentional.
What looks good
- Reuses the existing warn_if_unit_mismatch helper instead of reinventing the check. The helper was tested in isolation but unused; this wires it to a real call site.
- The warning runs on eager DataArray metadata before backend dispatch, so all four backends behave identically and no array is materialized.
- Tests cover both directions: the warning fires for degree coords with meter elevation, and stays silent for projected/meter coords. The no-warn test uses simplefilter("error", UserWarning), so any stray warning would fail it too.
- Docstring Notes section explains the failure mode and points to the fix (reproject or use geodesic).
Checklist
- Algorithm matches reference: n/a, no algorithm change
- All implemented backends consistent: yes, check runs pre-dispatch
- NaN handling: unchanged
- Edge cases covered by tests: degree and projected cases both covered
- Dask chunk boundaries: unchanged, warning is metadata-only
- No premature materialization: confirmed, helper samples metadata only
- Benchmark: not needed
- README feature matrix: not needed, no new function
- Docstrings present and accurate: yes
# Conflicts: # xrspatial/tests/test_slope.py
brendancol
commented
Jun 2, 2026
Contributor
Author
brendancol
left a comment
brendancol
left a comment
There was a problem hiding this comment.
PR Review: Warn on degree coords + meter elevation in slope planar path
Blockers
None.
Suggestions
None.
Nits
None.
What looks good
- Reuses the existing
warn_if_unit_mismatchhelper (utils.py:833) instead of writing fresh detection logic, called at the top of the planar branch (slope.py:402) before the resolution gets used. - Scoped to the planar path only. Geodesic is left alone, which is right since it handles lat/lon itself.
- The check reads coordinate metadata, so it fires once and never triggers a dask compute. That also makes it backend-agnostic.
- Tests cover both directions: degree coords + meter elevation warns, projected meter coords stay silent under
simplefilter("error", UserWarning). - The merge with main is clean. The test rasters use evenly spaced coords, so the separately-merged irregular-spacing warning (#2784) doesn't fire and trip the no-warn test. Full test_slope.py passes (67 tests).
- A Notes section in the slope docstring spells out the failure mode.
Checklist
- No algorithm change (adds a runtime warning)
- Backend-agnostic (warning runs on metadata)
- NaN handling unchanged by this PR
- Edge cases tested (degree warn, projected no-warn)
- No neighborhood/dask change
- Metadata-only check, no materialization
- No benchmark needed (no compute change)
- README matrix N/A (no new function, no backend change)
- Docstring Notes section added
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #2764
The planar slope path uses coordinate spacing directly as the cell size. When coordinates are in degrees (lat/lon) but elevation is in meters, the rise-over-run mixes units and the output is wrong by orders of magnitude, silently. A warning helper for this case already existed (
warn_if_unit_mismatchin utils.py) but nothing called it.warn_if_unit_mismatchinto the planar branch ofslope()so aUserWarningfires when degree-like coordinates meet meter-like elevation.slopedocstring.No new computation, only a runtime check on the existing planar path, so all four backends (numpy / cupy / dask+numpy / dask+cupy) are unaffected. The warning runs on the eager DataArray metadata before dispatch.
Test plan:
test_slope.pysuite passes (63 tests).